[Fiber] Refactor Commit Phase into Separate Functions for Before Mutation/Mutation/Layout #31930

sebmarkbage · 2024-12-28T08:02:16Z

Stacked on #31922.

This is doing some general clean up to be able to split the commit root three phases into three separate async steps.

vercel · 2024-12-28T08:02:21Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
react-compiler-playground	❌ Failed (Inspect)			Jan 2, 2025 7:46pm

react-sizebot · 2024-12-28T08:05:20Z

Comparing: d8b903f...e953c4e

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.js	=	6.68 kB	6.68 kB	=	1.83 kB	1.83 kB
oss-stable/react-dom/cjs/react-dom-client.production.js	+0.15%	511.67 kB	512.42 kB	+0.26%	91.36 kB	91.59 kB
oss-experimental/react-dom/cjs/react-dom.production.js	=	6.69 kB	6.69 kB	=	1.83 kB	1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js	+0.15%	516.43 kB	517.20 kB	+0.26%	92.21 kB	92.45 kB
facebook-www/ReactDOM-prod.classic.js	+0.19%	593.12 kB	594.22 kB	+0.25%	104.42 kB	104.67 kB
facebook-www/ReactDOM-prod.modern.js	+0.19%	583.38 kB	584.49 kB	+0.25%	102.86 kB	103.13 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
facebook-www/ReactART-prod.modern.js	+0.79%	354.31 kB	357.10 kB	+0.72%	59.38 kB	59.81 kB
facebook-www/ReactART-prod.classic.js	+0.77%	364.03 kB	366.83 kB	+0.74%	60.95 kB	61.41 kB
oss-stable-semver/react-art/cjs/react-art.production.js	+0.39%	299.93 kB	301.11 kB	+0.15%	51.21 kB	51.29 kB
oss-stable/react-art/cjs/react-art.production.js	+0.39%	300.00 kB	301.19 kB	+0.15%	51.24 kB	51.32 kB
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-prod.js	+0.35%	332.31 kB	333.48 kB	+0.16%	58.15 kB	58.24 kB
facebook-www/ReactART-dev.modern.js	+0.34%	636.86 kB	639.04 kB	+0.30%	100.95 kB	101.25 kB
oss-experimental/react-art/cjs/react-art.production.js	+0.34%	304.53 kB	305.56 kB	+0.17%	52.12 kB	52.21 kB
facebook-www/ReactART-dev.classic.js	+0.34%	646.74 kB	648.92 kB	+0.31%	102.90 kB	103.22 kB
react-native/implementations/ReactFabric-prod.js	+0.33%	356.03 kB	357.20 kB	+0.10%	62.07 kB	62.13 kB
react-native/implementations/ReactNativeRenderer-prod.js	+0.32%	364.24 kB	365.41 kB	+0.14%	63.38 kB	63.47 kB
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-profiling.js	+0.32%	354.33 kB	355.45 kB	+0.20%	61.24 kB	61.36 kB
react-native/implementations/ReactFabric-prod.fb.js	+0.31%	380.73 kB	381.90 kB	+0.10%	66.33 kB	66.40 kB
react-native/implementations/ReactNativeRenderer-prod.fb.js	+0.30%	386.17 kB	387.34 kB	+0.07%	67.30 kB	67.34 kB
oss-stable-semver/react-reconciler/cjs/react-reconciler.production.js	+0.25%	387.46 kB	388.41 kB	+0.20%	62.91 kB	63.04 kB
oss-stable/react-reconciler/cjs/react-reconciler.production.js	+0.25%	387.48 kB	388.44 kB	+0.19%	62.94 kB	63.06 kB
facebook-www/ReactReconciler-prod.modern.js	+0.23%	448.43 kB	449.48 kB	+0.17%	72.15 kB	72.28 kB
react-native/implementations/ReactFabric-profiling.js	+0.23%	381.42 kB	382.30 kB	+0.18%	65.72 kB	65.84 kB
facebook-www/ReactReconciler-prod.classic.js	+0.23%	458.75 kB	459.80 kB	+0.18%	73.71 kB	73.84 kB
react-native/implementations/ReactNativeRenderer-profiling.js	+0.22%	389.67 kB	390.54 kB	+0.18%	67.13 kB	67.25 kB
react-native/implementations/ReactFabric-profiling.fb.js	+0.22%	406.37 kB	407.25 kB	+0.06%	70.22 kB	70.26 kB
react-native/implementations/ReactNativeRenderer-profiling.fb.js	+0.21%	411.78 kB	412.65 kB	+0.05%	71.10 kB	71.13 kB
oss-experimental/react-reconciler/cjs/react-reconciler.production.js	+0.20%	392.64 kB	393.43 kB	+0.18%	63.85 kB	63.97 kB

Generated by 🚫 dangerJS against f472472

sebmarkbage · 2024-12-28T16:29:54Z

packages/react-reconciler/src/__tests__/ReactDeferredValue-test.js

@@ -744,6 +744,7 @@ describe('ReactDeferredValue', () => {
        </Container>,
      );
      // We should switch to pre-rendering the new preview.
+      await waitForPaint([]);


This happens because the requestPaint() is now unconditional even if there weren't any effects. Which doesn't really hurt anything since it's actually a noop in anything but tests and should probably go away.

sebmarkbage · 2024-12-29T01:02:43Z

packages/react-reconciler/src/ReactFiberWorkLoop.js

@@ -3460,7 +3471,8 @@ function commitRootImpl(
    }
  }

-  onCommitRootDevTools(finishedWork.stateNode, renderPriorityLevel);


This one is a bit strange because the profiler in devtools tracks the priority level that the commit phase is running in rather than what priority was rendered and then committed. I think the idea is that it would be running in the scheduler priority of the render but that's not necessarily the case for the commit.

DuoAlly-AI-Recruiter

All the codes function correctly.

anukaal

looks ok

rickhanlonii

The refactor LGTM but also changing the finishedWork to cancelPendingCommit seems sus. Any way you could at least break that change into a separate PR so it's easier to revert if there's an unexpected issue?

rickhanlonii · 2025-01-02T18:50:23Z

packages/react-reconciler/src/ReactFiberWorkLoop.js

@@ -1839,9 +1841,6 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes): Fiber {
    }
  }

-  root.finishedWork = null;
-  root.finishedLanes = NoLanes;


Should root.cancelPendingCommit and timeoutHandle get reset?

They do get reset right below here as they're cancelled.

rickhanlonii · 2025-01-02T18:51:04Z

packages/react-reconciler/src/ReactFiberWorkLoop.js

            'bug in React.',
        );
      }
    }
  }
-  root.finishedWork = null;
-  root.finishedLanes = NoLanes;


Should root.cancelPendingCommit and timeoutHandle get reset?

root.cancelPendingCommit gets reset in the beginning of commitRoot. timeoutHandle gets reset in the beginning of commitRootWhenReady.

Which is really just right at the beginning of the callback that was pending.

The idea is that it's reset right after the callback is fired, at the very beginning, since it's no longer pending. Or it gets reset when it's cancelled.

rickhanlonii · 2025-01-02T18:53:15Z

packages/react-reconciler/src/ReactFiberWorkLoop.js

+    }
+  }
+  flushMutationEffects(root, finishedWork, lanes);
+  flushLayoutEffects(


Might be nice to move the before mutation logic above to flushBeforeMutationEffects to match these.

These won't remain exactly like this once they get split out into the async ones and there's more phases a bit in motion. For now it's to make it clear which parts are sync and which parts are async.

Once that settles we can see where we end up.

rickhanlonii · 2025-01-02T18:54:52Z

packages/react-reconciler/src/ReactFiberWorkLoop.js

+    const prevTransition = ReactSharedInternals.T;
+    const previousUpdateLanePriority = getCurrentUpdatePriority();
+    setCurrentUpdatePriority(DiscreteEventPriority);
+    ReactSharedInternals.T = null;


Why do we need to set the priority here? Is this just so the reactor is more 1-1?

Yea it's mainly to keep it 1-1. Since this is a user space callback you could have a setState in here such as updating an error dialog.

Therefore this must have some semantic. It used to be Discrete based on how it was factored but it makes sense that it would be.

Whether it always should be anyway is an interesting question though. It really shouldn't be possible to run a commit inside a startTransition so I'm not sure how it could ever be anything else.

We're already setting the update priority around user space code.

The execution scope of the commit isn't as useful for profiling information.

This aligns it with passive effects. We can cancel these using the timeoutHandle or cancelPendingCommit. We should never run a commit function that was cancelled. We can check these to determine whether there are any pending commits.

Because ReactFiberLane is imported for its constants it cannot depend on the config.

sebmarkbage · 2025-01-02T19:55:19Z

The refactor LGTM but also changing the finishedWork to cancelPendingCommit

This part gives me a bit more confident that it'll work correctly with the async gaps inserted. I think we'll just have to revert this whole PR if something goes wrong. Which is why I wanted to land this piece as early as possible.

sebmarkbage · 2025-01-02T19:57:16Z

There's still an outstanding thing to resolve. It doesn't show up in this PR since it's still sync but once async gaps are inserted there could be setStates/renders that in the gap that needs to model a pending commit similar to passive but it's more like Execution Environment in that all sync updates need to be deferred until this commit finishes.

…tion/Mutation/Layout (#31930) This is doing some general clean up to be able to split the commit root three phases into three separate async steps. DiffTrain build for [c81312e](c81312e)

sebmarkbage requested review from rickhanlonii and acdlite December 28, 2024 08:02

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Dec 28, 2024

vercel bot deployed to Preview December 28, 2024 08:03 View deployment

sebmarkbage commented Dec 28, 2024

View reviewed changes

sebmarkbage commented Dec 29, 2024

View reviewed changes

DuoAlly-AI-Recruiter reviewed Dec 31, 2024

View reviewed changes

anukaal reviewed Jan 2, 2025

View reviewed changes

rickhanlonii approved these changes Jan 2, 2025

View reviewed changes

sebmarkbage added 7 commits January 2, 2025 14:35

Merge commitRoot and commitRootImpl

cd938a8

We're already setting the update priority around user space code.

Compute priority of the commit from the lanes that committed

1abd933

The execution scope of the commit isn't as useful for profiling information.

Pass rootHasPendingCommit as argument

2b27ede

Because ReactFiberLane is imported for its constants it cannot depend on the config.

Ensure the handle gets reset right after being invoked

f89bb93

Split before mutation, mutation and layout phases

24225bd

Split mutation effects and layout effects into separate functions

f472472

sebmarkbage force-pushed the refactorcommitphase branch from facd6aa to f472472 Compare January 2, 2025 19:45

vercel bot had a problem deploying to Preview January 2, 2025 19:46 Failure

sebmarkbage merged commit c81312e into facebook:main Jan 2, 2025
186 of 187 checks passed

vercel-release-bot mentioned this pull request Jan 3, 2025

Upgrade React from 518d06d2-20241219 to 3b009b4c-20250102 vercel/next.js#74492

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fiber] Refactor Commit Phase into Separate Functions for Before Mutation/Mutation/Layout #31930

[Fiber] Refactor Commit Phase into Separate Functions for Before Mutation/Mutation/Layout #31930

sebmarkbage commented Dec 28, 2024

vercel bot commented Dec 28, 2024 •

edited

Loading

react-sizebot commented Dec 28, 2024 •

edited

Loading

sebmarkbage Dec 28, 2024

sebmarkbage Dec 29, 2024

DuoAlly-AI-Recruiter left a comment

anukaal left a comment

rickhanlonii left a comment

rickhanlonii Jan 2, 2025

sebmarkbage Jan 2, 2025

rickhanlonii Jan 2, 2025

sebmarkbage Jan 2, 2025

rickhanlonii Jan 2, 2025

sebmarkbage Jan 2, 2025

rickhanlonii Jan 2, 2025

sebmarkbage Jan 2, 2025

sebmarkbage commented Jan 2, 2025 •

edited

Loading

sebmarkbage commented Jan 2, 2025

[Fiber] Refactor Commit Phase into Separate Functions for Before Mutation/Mutation/Layout #31930

[Fiber] Refactor Commit Phase into Separate Functions for Before Mutation/Mutation/Layout #31930

Conversation

sebmarkbage commented Dec 28, 2024

vercel bot commented Dec 28, 2024 • edited Loading

react-sizebot commented Dec 28, 2024 • edited Loading

Critical size changes

Significant size changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DuoAlly-AI-Recruiter left a comment

Choose a reason for hiding this comment

anukaal left a comment

Choose a reason for hiding this comment

rickhanlonii left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebmarkbage commented Jan 2, 2025 • edited Loading

sebmarkbage commented Jan 2, 2025

vercel bot commented Dec 28, 2024 •

edited

Loading

react-sizebot commented Dec 28, 2024 •

edited

Loading

sebmarkbage commented Jan 2, 2025 •

edited

Loading